Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed implementation of RichTextLabel remove_line(), which fixed issues in EditorLog. #49060

Merged
merged 1 commit into from
May 26, 2021
Merged

Fixed implementation of RichTextLabel remove_line(), which fixed issues in EditorLog. #49060

merged 1 commit into from
May 26, 2021

Conversation

EricEzaM
Copy link
Contributor

@EricEzaM EricEzaM commented May 25, 2021 •
edited
Loading

There were some issues in RichTextLabel remove_line() method, where items were not correctly removed, and line decremending for items in later lines was not correctly done.

This also fixed several headaches with EditorLog, which relied on the remove_line() method for collapsing of duplicate messages.

I added some comments which should help clarify some of the logic for the next person to look at this code.

Fixes #49030

See #49030 for the code which I used for the below samples. Essentially the spacebar just calls remove_line(get_line_count() - 1), which should remove the last line of the rich text label. I also tested the update with code which removes lines from the middle of a set of lines and it worked as expected.

Note the "Before" was even more broken because an update() was not triggered on remove_line() - for the below "Before" showcase I added a manual call to update() in the GDScript code.

BEFORE

RTL_BROKEN_example.mp4

AFTER

RTL_FIXED_example.mp4

Editor Log AFTER

RTL_FIXED_editorlog.mp4

CC @KoBeWi I hope you can stop submitting reports about EditorLog now... 😂

@EricEzaM EricEzaM requested a review from a team as a code owner May 25, 2021 12:07
@EricEzaM EricEzaM changed the title Fixed implementation of RTL remove_line(), which fixed issues in EditorLog. Fixed implementation of RichTextLabel remove_line(), which fixed issues in EditorLog. May 25, 2021
@EricEzaM EricEzaM added this to the 4.0 milestone May 25, 2021
@KoBeWi
Copy link
Member

KoBeWi commented May 25, 2021

CC @KoBeWi I hope you can stop submitting reports about EditorLog now... 😂

Haha, you wish XD

Printing is still broken, but only after starting.

func _process(delta):
	print("test")

UtX4Gb7QzQ
Clearing the text fixes it permanently.

@EricEzaM
Copy link
Contributor Author

Uhhh... Progress...? 😅

@EricEzaM EricEzaM force-pushed the fix-rich-text-label-and-editor-log branch from 7cca786 to 9003326 Compare May 25, 2021 23:16
@EricEzaM
Copy link
Contributor Author

EricEzaM commented May 25, 2021 •
edited
Loading

More issues with RichTextLabel... see #45128 (comment)

get_line_count() was not counting the number of lines internally in RichTextLabel, but all rendered lines... So if there was long enough text to go over multiple lines, it would report more lines than was is current_frame->lines.size(), which is what remove_line checks against when it removes lines. I have added a new separate commit to this PR to add an optional parameter to get_line_count() which allows optionally including wrapped lines as multiple lines or as one line.

cc @KoBeWi @bruvzg

See the below video, I highlight the lines which wrap. These increased the value of get_line_count() by 2, so when I called remove_line(get_line_count() - 2), it returned early since the index was out of bounds of lines.size()

RTL_LOG_FIXED.mp4

@EricEzaM EricEzaM force-pushed the fix-rich-text-label-and-editor-log branch from 9003326 to a8f468d Compare May 26, 2021 00:30
@EricEzaM EricEzaM requested a review from a team as a code owner May 26, 2021 00:30
@EricEzaM
Fixed implementation of RTL remove_line(), which fixed issues in Edit…
471f7f1 
…orLog.

There were some issues in RichTextLabel `remove_line()` method, where items were not correctly removed, and line decremending for items in later lines was not correctly done.
This also fixed several headaches with EditorLog, which relied on the `remove_line()` method for collapsing of duplicate messages. The fix to RTL also fixed the issues with EditorLog.

Fixes godotengine#49030
@EricEzaM EricEzaM force-pushed the fix-rich-text-label-and-editor-log branch from a8f468d to 471f7f1 Compare May 26, 2021 05:08
@EricEzaM
Copy link
Contributor Author

Ok, turns out what I was after was get_paragraph_count() which is what get_line_count() used to be.

Updated

KoBeWi
KoBeWi approved these changes May 26, 2021
View reviewed changes
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the bug is fixed... for now.

@akien-mga akien-mga merged commit ca4d2ff into godotengine:master May 26, 2021
@akien-mga
Copy link
Member

Thanks!

@EricEzaM
Copy link
Contributor Author

EricEzaM commented May 26, 2021 •
edited
Loading

@KoBeWi have some confidence 😭 😛

@EricEzaM EricEzaM deleted the fix-rich-text-label-and-editor-log branch October 5, 2021 12:23
This was referenced Apr 29, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga approved these changes

@KoBeWi KoBeWi KoBeWi approved these changes

Assignees
No one assigned
Labels
bug topic:editor topic:gui
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

Printed messages don't get grouped
3 participants
@EricEzaM @KoBeWi @akien-mga